Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The ability to allocation and perform further initialization of large PE files. #198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nitr0-G
Copy link

@Nitr0-G Nitr0-G commented Feb 4, 2024

Added the ability to read files larger than 40MB at a time. This is necessary, for example, for further initialization of a large PE file, for example, its Imports and the like, which requires a full file in memory. One bool variable was added with default initialization as false, so as not to disrupt the work of projects on older versions or when switching to a new version. The file is loaded by linking two APIs: VirtualAlloc + ReadFile. Thanks for such a wonderful pe parser like this =)

Added the ability to read files larger than 40MB at a time. This is necessary, for example, for further initialization of a large PE file, for example, its Imports and the like, which requires a full file in memory. One bool variable was added with default initialization as false, so as not to disrupt the work of projects on older versions or when switching to a new version. The file is loaded by linking two APIs: VirtualAlloc + ReadFile. Thanks for such a wonderful pe parser like this =)
@Nitr0-G Nitr0-G requested a review from woodruffw as a code owner February 4, 2024 00:23
@CLAassistant
Copy link

CLAassistant commented Feb 4, 2024

CLA assistant check
All committers have signed the CLA.

Comment on lines +256 to +277

LPVOID ptr = nullptr;

if (!LargeFile) {
ptr = MapViewOfFile(hMap, FILE_MAP_READ, 0, 0, 0);
} else {
ptr = VirtualAlloc(NULL, fileSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
if (ptr == INVALID_HANDLE_VALUE) {
CloseHandle(h);
CloseHandle(ptr);
return nullptr;
}

const bool bFileRead = ReadFile(h, ptr, fileSize, nullptr, nullptr);
if(!bFileRead) {
CloseHandle(h);
if (ptr != nullptr) {
CloseHandle(ptr);
}

return nullptr;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it only uses a mmap backing on Windows, but we'll probably want similar behavior on other OSes as well. Could you add that to this changeset?

@@ -195,7 +195,7 @@ std::string GetPEErrString();
std::string GetPEErrLoc();

// get a PE parse context from a file
parsed_pe *ParsePEFromFile(const char *filePath);
parsed_pe *ParsePEFromFile(const char *filePath, bool LargeFile = false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a weak -1 on modifying the API like this: IMO we should pick an arbitrary-ish size for the "large file" cutoff, and use stat or ftell or similar to determine when to switch over to it based on the user input.

(We could do this via a macro, so users who compile pe-parse themselves could customize it.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32MB seems like a reasonable starting point for the cutoff.

@woodruffw
Copy link
Member

Thanks for these changes @Nitr0-G! I've given them an initial review pass and left some thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants